Skip to content

Conversation

tspiteri
Copy link
Contributor

@tspiteri tspiteri commented Jul 7, 2021

Now that #80918 has been merged, this PR provides a faster version of log10.

The PR also adds some tests for values close to all powers of 10.

@rust-highfive
Copy link
Contributor

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 7, 2021
@tspiteri
Copy link
Contributor Author

tspiteri commented Jul 7, 2021

Some timings of old versus new:

test u128_new ... bench:       8,793 ns/iter (+/- 509)
test u128_old ... bench:      28,489 ns/iter (+/- 892)
test u16_new  ... bench:         259 ns/iter (+/- 12)
test u16_old  ... bench:         921 ns/iter (+/- 27)
test u32_new  ... bench:         718 ns/iter (+/- 21)
test u32_old  ... bench:       1,691 ns/iter (+/- 25)
test u64_new  ... bench:         848 ns/iter (+/- 23)
test u64_old  ... bench:       4,221 ns/iter (+/- 95)
test u8_new   ... bench:         219 ns/iter (+/- 8)
test u8_old   ... bench:         371 ns/iter (+/- 6)
Benchmark details

macro_rules! bench {
    ($T:ident, $old:ident, $new:ident) => {
        // for example for u32, compute:
        //   * 1, 2, 3, ..., 255
        //   * 1 << 8, 2 << 8, 3 << 8, ..., 255 << 8
        //   * 1 << 16, 2 << 16, 3 << 16, ..., 255 << 16
        //   * 1 << 24, 2 << 24, 3 << 24, ..., 255 << 24

        #[bench]
        fn $old(bench: &mut Bencher) {
            for n in 0..(<$T>::BITS / 8) {
                bench.iter(|| {
                    for i in 1..=(u8::MAX as $T) {
                        let i = i << (n * 8);
                        black_box(old::$T(i, 10));
                    }
                });
            }
        }

        #[bench]
        fn $new(bench: &mut Bencher) {
            for n in 0..(<$T>::BITS / 8) {
                bench.iter(|| {
                    for i in 1..=(u8::MAX as $T) {
                        let i = i << (n * 8);
                        black_box(int_log10::$T(i));
                    }
                });
            }
        }
    };
}

bench! { u8, u8_old, u8_new }
bench! { u16, u16_old, u16_new }
bench! { u32, u32_old, u32_new }
bench! { u64, u64_old, u64_new }
bench! { u128, u128_old, u128_new }

@rust-log-analyzer

This comment has been minimized.

@kennytm
Copy link
Member

kennytm commented Jul 7, 2021

@bors rollup=never

@kennytm
Copy link
Member

kennytm commented Jul 7, 2021

@bors r+

@bors
Copy link
Collaborator

bors commented Jul 7, 2021

📌 Commit ed76c11 has been approved by kennytm

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 7, 2021
@bors
Copy link
Collaborator

bors commented Jul 7, 2021

⌛ Testing commit ed76c11 with merge c014ff7e6d366543042d6b70e40d023cf27dbdf7...

@bors
Copy link
Collaborator

bors commented Jul 7, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 7, 2021
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@tspiteri
Copy link
Contributor Author

tspiteri commented Jul 7, 2021

The failure seems spurious, or am I missing something?

1
} else {
0
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tried doing a binary search instead a linear chain of inequalities? I'm not sure if it would actually be faster due to unpredictable branches.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did try it and it seemed to be slightly slower (just over the error thresholds), so I thought it better to leave the simpler code since there was no clear gain.

@the8472
Copy link
Member

the8472 commented Jul 7, 2021

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 7, 2021
@bors
Copy link
Collaborator

bors commented Jul 7, 2021

⌛ Testing commit ed76c11 with merge 8879bfb0f219936a8255549ba640b7f2f40b793d...

@bors
Copy link
Collaborator

bors commented Jul 8, 2021

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 8, 2021
@JohnTitor
Copy link
Member

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 8, 2021
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@bors
Copy link
Collaborator

bors commented Jul 8, 2021

⌛ Testing commit ed76c11 with merge 3f75f8508a00f5046784fa6cec436c926ab6a1b8...

@bors
Copy link
Collaborator

bors commented Jul 8, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 8, 2021
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@JohnTitor
Copy link
Member

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 8, 2021
@bors
Copy link
Collaborator

bors commented Jul 8, 2021

⌛ Testing commit ed76c11 with merge 8b87e85...

@bors
Copy link
Collaborator

bors commented Jul 8, 2021

☀️ Test successful - checks-actions
Approved by: kennytm
Pushing 8b87e85 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 8, 2021
@bors bors merged commit 8b87e85 into rust-lang:master Jul 8, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jul 8, 2021
@tspiteri tspiteri deleted the int_log10 branch August 19, 2021 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants